Implement new pganalyze.explain_analyze() helper #655
Conversation
|
I think overall this is looking good (I admit that I haven't checked the SQL part). |
Yeah, I was going to remove the flag in a different PR. And good point re: the helper - though technically we can also get that via the schema function data that already gets reported. The main reason to stay with that existing flow that I could see (vs also including it in collector info), is that this is a per-database helper, like the Index Advisor helpers. Storing this information in collector info for many databases might not be ideal? |
Ah, interesting, so you'll need to create a helper function to all databases (if you have many databases). Yeah in that case, sending out in the collector info doesn't make sense. I think I can workaround by checking the function definition on the app side 👍 |
lfittl
force-pushed
the
explain-function-helper
branch
2 times, most recently
from
0771dbe to
c61e0e4
Compare
| TEST_DATABASE_URL: postgresql://postgres:postgres@localhost:5432/postgres?sslmode=disable | ||
| services: | ||
| postgres: | ||
| image: postgres:14 |
There was a problem hiding this comment.
Using 14 as a minimum here since the EXPLAIN ANALYZE tests rely on being able to turn off compute_query_id in the output (and that was added in 14).
|
@keiko713 @seanlinsley FYI, this is ready for re-review. I ended up adding both parameter support and Postgres setting support here as well, since it was quick enough, and even if the server doesn't use it that works fine. |
lfittl
force-pushed
the
explain-function-helper
branch
4 times, most recently
from
dc15ca0 to
f192625
Compare
|
@seanlinsley @keiko713 This is ready for re-review, all feedback should be addressed. |
| @@ -130,6 +130,34 @@ func Run(ctx context.Context, wg *sync.WaitGroup, globalCollectionOpts state.Col | |||
| return | |||
| } | |||
|
|
|||
| if globalCollectionOpts.GenerateExplainAnalyzeHelperSql != "" { | |||
| wg.Add(1) | |||
| testRunSuccess = make(chan bool) | |||
There was a problem hiding this comment.
It doesn't have to be part of this PR, but we may want to add test run output for the presence of the helper function and success on running a test explain.
There was a problem hiding this comment.
Yeah, that's a good point. I'll add this in a follow-up PR.
| PERFORM 1 FROM pg_roles WHERE (rolname = current_user AND rolsuper) OR (pg_has_role(oid, 'MEMBER') AND rolname IN ('rds_superuser', 'azure_pg_admin', 'cloudsqlsuperuser')); | ||
| IF FOUND THEN | ||
| RAISE EXCEPTION 'cannot run: helper is owned by superuser - recreate function with lesser privileged user'; | ||
| END IF; |
There was a problem hiding this comment.
This error occurs in local testing because my Postgres user is a superuser. Maybe the OR should be an AND instead?
For reference, these are the values for the different where clauses:
select rolsuper, pg_has_role(oid, 'MEMBER'), rolname IN ('rds_superuser', 'azure_pg_admin', 'cloudsqlsuperuser') from pg_roles where rolname = current_user;
rolsuper | pg_has_role | ?column?
----------+-------------+----------
t | t | fIf the logic in this check is intentional, what should I do to my local Postgres install to work around it, and how should we communicate this issue in the docs?
There was a problem hiding this comment.
If you follow the instructions on setting up the separate user (preview) it should work as expected, even in a local setup. Its intentionally an OR, since we want to block users that run Postgres on a VM to use a real superuser for this.
|
This looks good overall, but I did run into one bug: the statement timeout doesn't seem to take effect. For example if I run a |
Strange - I had tested that and it worked for me, but I can recheck tomorrow. It sounds like you got an actual result vs the EXPLAIN without ANALYZE that should happen when the query times out. |
Some time ago (at least 9+ years) we introduced a maximum connection lifetime of 30 seconds via the SetConnMaxLifetime database/sql function. This was likely intended as an extra safety measure in case the collector didn't clean up its DB handle correctly. We've had a few minor issues with incorrect cleanups over the years (though usually not with the DB connection), but at this point our connection cleanup should be reliable. The connection lifetime causes an issue because it causes Go to reconnect to the database after 30 seconds have elapsed. This then effectively removes any connection local settings, in particular a statement_timeout. Effectively this meant that queries may unintentionally not have been subject to the statement_timeout, if the same connection previously took 30 seconds to do its work. To resolve, remove the max lifetime.
This executes on-demand query runs for the Query Tuning feature via a helper function that prevents reading the table data directly, as well as granting the collector user unnecessary permissions. This is now the only way that query runs can execute EXPLAIN ANALYZE, direct execution without the helper is intentionally not supported. In passing, expand the protobufs to pass down parameters and parameter types and optional planner settings, and refactor the query run code to handle work requiring a DB connection in a separate Go function. Further, this adds tests that utilize a new TEST_DATABASE_URL enviroment setting, which is used to run unit tests that require a database connection (previously only the integration tests used the database).
Like "--generate-stats-helper-sql" this allows generating a SQL script that installs the pganalyze.explain_analyze helper into all databases on a server, as specified by the server section name passed as an argument. Additionally, the "--generate-helper-explain-analyze-role" option allows setting the role that the helper function should be owned by as a value, defaulting to "pganalyze_explain". Note that the role creation, as well as any GRANT statements for the role must be taken care of separately.
lfittl
force-pushed
the
explain-function-helper
branch
from
5e7d30f to
bd904f4
Compare
This executes on-demand query runs for the Query Tuning feature via
a helper function that prevents reading the table data directly, as well
as granting the collector user unnecessary permissions.
This is now the only way that query runs can execute EXPLAIN ANALYZE,
direct execution without the helper is intentionally not supported.